-
Notifications
You must be signed in to change notification settings - Fork 7
YDBOPS-12647: New inventory and some fixes #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
plugins/inventory/ydb_inventory.py
Outdated
|
|
||
| self.inventory.groups['ydb'].set_variable('ydb_config_dict', yaml_config) | ||
|
|
||
| if 'ydb_dbname' not in ydb_vars and 'ydb_dynnodes' in ydb_vars: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кажется здесь плагин решает какую-то проблему определения дефолтов, которую inventory плагин не должен решать. Для этого у ansible есть другие механизмы. Особенно учитывая, что конфиг никак не используется для определения этих дефолтов
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А вот здесь возникает интересная проблема:
- inventory плагин анализирует config.yaml и, в частности, сопоставляет диски хостам
- в inventory.yaml файле прописывается соответствие дисков label-ам
- если плагин запускать до загрузки inventory.yaml, то у нас не будет распределения дисков с лейблами по хостам, либо надо делать еще один плагин, который будет срабатывать после парсинга инвентори, те надо будет 3 файла уже в каталоге inventory
можно расширить config.yaml и в него тогда внести описание дисковых лейблов, но это будет дополнительные поля, на которые будет ругаться парсер YDB, либо надо уходить от лейблов
лучше, на мой взгляд, оставить так как сейчас реализовано
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
изменено поведение
playbooks/format_cluster.yaml
Outdated
| become: true | ||
| shell: dd if=/dev/zero of={{ item.name }} bs=1M count=100 status=none | ||
| loop: "{{ ydb_disks }}" | ||
| shell: dd if=/dev/zero of={{ item.name | default(ydb_disks|selectattr('label','match',item.label)|map(attribute='name')|join()) }} bs=1M count=100 status=none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Свалится с неочевидной для пользователя ошибкой, если в ydb_disks нет диска с лейблом ydb_drives[x].label.
Будет попытка запустить dd if=/dev/zero of= bs=1M count=100 status=none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добавлена проверка дисков
playbooks/format_drives.yaml
Outdated
| become: true | ||
| shell: dd if=/dev/zero of={{ item.name }} bs=1M count=100 status=none | ||
| loop: "{{ ydb_disks }}" | ||
| shell: dd if=/dev/zero of={{ item.name | default(ydb_disks|selectattr('label','match',item.label)|map(attribute='name')|join()) }} bs=1M count=100 status=none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Свалится с неочевидной для пользователя ошибкой, если в ydb_disks нет диска с лейблом ydb_drives[x].label.
Будет попытка запустить dd if=/dev/zero of= bs=1M count=100 status=none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добавлена проверка дисков
filter_plugins/yaml_filters.py
Outdated
| DOCUMENTATION = r''' | ||
| name: yaml_filters | ||
| plugin_type: filter | ||
| short_description: short desc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эти описания-заглушки здесь ожидаемы?
plugins/inventory/ydb_inventory.py
Outdated
| self.inventory.add_group(group_name) | ||
| brokers = [] | ||
|
|
||
| ydb_vars = self.inventory.groups[group_name].get_vars() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Идея была никак не полагаться на состояние переменных в момент вызова плагина.
Первая же идея, как такое сломается: пользователь разбирается в ansible и захочет определить для отдельного хоста или подмножествва хостов свои ydb_disks.
В этом плагине ты потеряешь эти настройки пользователя.
Что будет, если в момент вызова плагина еще не успели определиться какие-то переменные?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Изменено поведение плагина
plugins/inventory/ydb_inventory.py
Outdated
| drive_configs = {} | ||
| drive_labels = {} | ||
|
|
||
| if 'ydb_disks' in ydb_vars: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Идея была никак не полагаться на состояние переменных в момент вызова плагина.
Первая же идея, как такое сломается: пользователь разбирается в ansible и захочет определить для отдельного хоста или подмножествва хостов свои ydb_disks.
В этом плагине ты потеряешь эти настройки пользователя.
Что будет, если в момент вызова плагина еще не успела определиться ydb_disks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Изменено поведение плагина
plugins/inventory/ydb_inventory.py
Outdated
| for drive_config in yaml_config['host_configs']: | ||
| drive_configs[drive_config['host_config_id']] = copy.deepcopy(drive_config['drive']) | ||
| for i, item in enumerate(drive_config['drive']): | ||
| label = item['path'].split('/')[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что делать, если пользователь инвентори не хочет пользоваться метками, а использовать другие механизмы обеспечения стабильных имен дисков? Что делать, если последний компонент пути диска не является лейблом?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добавлена проверка дисков, которая учитывает эти моменты
plugins/inventory/ydb_inventory.py
Outdated
| env: | ||
| - name: INVENTORY_YDB_CONFIG | ||
| ydb_hostgroup_name: | ||
| description: The name of group of hosts (default name is 'ydb') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно написать значение по умолчанию в отдельное поле default.
При этом, кажется, не придется определять его в коде, self.get_option('ydb_hostgroup_name') должен автоматически вернуть дефолт из документации, если пользователь не определил параметр (стоит перепроверить)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Доработано описание
roles/ydbd_static/tasks/main.yaml
Outdated
| ydb_platform.ydb.drive_prepare: | ||
| name: "{{ item['name'] }}" | ||
| name: "{{ item.name | default(ydb_disks|selectattr('label','match',item.label)|map(attribute='name')|join()) }}" | ||
| label: "{{ item['label'] }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В этом месте пустое значение допустимо, так как модуль drive_prepare сделает проверку и вылетит с ошибкой, если такое встретит
ydbas user/group names into variables for more flexibility.